chore(yun-tianming): share template poll reader, hoist cron refs lookup - #380

No issues found across 2 files

<sub>Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

LGTM - approved.

Summary

Two small, genuinely behaviour-preserving refactors — one readDeployment(api, projectId) helper replacing three byte-identical poll closures in template.ts, and a hoist of a pure refsOf call in cron.ts — both of which I reproduced and mutation-tested locally at 771bfb4; no Critical findings.

(Note: the PR was merged at 2026-10-17T10:11:30Z, mid-review, as 1398146. Everything below was verified against head 771bfb4ed98885ad3060f0680cdc6e6ad54f2d37, which is the merged tree.)

Requirements context

No matching spec/plan found — this repo has no docs/ directory at all (neither docs/superpowers/ nor the docs/specs/ the older InsForge/CLI repo used). .claude/skills/developing-insta-cli/SKILL.md, CONTRIBUTING.md and AGENTS.md carry no comment-style or dedup convention, so this is assessed against the PR description's own claims plus repo convention as practised in the surrounding code.

Verification performed

• npm ci clean, npx tsc --noEmit exit 0.
• npx vitest run at head: 106 files, 2243 passed, 1 skipped (the body reports 135 skips; this box has ssh/ssh-keygen, so the SSH suites actually run — more coverage than the author measured, and all green).
• Equivalence of the three collapsed closures checked directly: git show main:src/commands/template.ts lines 468/616/671 are byte-identical apart from the captured api/p.projectId, both of which are const in their enclosing functions and are never reassigned between binding and the watchDeployment call — so moving p.projectId from lazy (per-poll) to eager (at call time) evaluation is not observable.
• refsOf (src/commands/cron.ts:173-175) is r.secretRefs ?? {} — pure, no allocation of a fresh object per call for the non-null branch, so the hoist is a strict no-op on semantics and a strict (small) win on work done.
• Negative controls run on top of head:
  • drop { projectId } from readDeployment → 1 test red (template.test.ts, the pollScopes assertion at test/template.test.ts:752-753).
  • corrupt the poll path in readDeployment → 16 tests red.
  • refsOf → {} → 9 tests red in test/cron.test.ts.
  • readDeployment(api, 'WRONG_PROJECT') at the upgrade call site only (src/commands/template.ts:619) → full suite GREEN (see Suggestion 1).

Findings

Critical

(none)

Suggestion

Software engineering / test coverage — the upgrade and rollback poll scopes are unasserted; the deploy test is the only pin. src/commands/template.ts:619 and src/commands/template.ts:674. Replacing p.projectId with a wrong literal at the upgrade site alone leaves the entire suite green (106/106). test/template-upgrade.test.ts executes those poll paths but its fake api asserts only on posts[0].path (test/template-upgrade.test.ts:108,155) — it never captures the poll path or the opts scope the way fakeApi's pollScopes does in test/template.test.ts:716,752. This is pre-existing, not introduced here, and the collapse actually reduces the blast radius (one helper, one assertion). But it makes one sentence of the Safety section optimistic: test/template-upgrade.test.ts exercises those code paths, it does not pin them. Cheapest fix while the helper is fresh: have the upgrade fake record (path, opts) on GETs to /template-deployments/ and assert { projectId: 'proj_1' }, mirroring the deploy test.

Software engineering / completeness — one more identical refsOf repeat was left in place. src/commands/cron.ts:401: current.headerNames.filter((n) => … findHeader(refsOf(current), n) === undefined) calls refsOf once per header name, inside a filter callback — exactly the shape the PR hoists out of jobShowLines at line 233. Not a defect (pure, cheap), but the sweep's own stated standard ("the refs are now read once into a local") applies verbatim here, and leaving it means the next sweep re-finds it.

Information

Functionality — the relocated comment is accurate. The template.ts:387-388 comment now sits above readDeployment and states the constraint once; the rewritten comment at template.ts:576-579 correctly describes only the preview GET it still precedes (template.ts:582, which does pass { projectId: p.projectId } and does need its own 409 reading via templateErrorAdvice). No stale claim left behind — the failure mode my previous sweep reviews (#286, #365) kept hitting (a shrunk comment that drops a present-tense fact) does not occur here.

Security — no security-relevant changes in this PR. No new user input reaches SQL, shell or HTTP; the request path, method, body and scope are unchanged byte-for-byte; no secret, token or PII is newly logged or returned (cron.ts's header rendering still prints names only and secret names, never values); no auth check touched — in fact the one auth-adjacent behaviour (naming the project so agent mode picks the project-bound session) is now harder to drop accidentally, since one helper serves all three call sites. No dependency changes.

Performance — net positive, trivially. cron.ts drops one refsOf call per header name per cron show (an O(headers) → O(1) change on a sub-millisecond pure function). template.ts is neutral: one closure allocated per command invocation, same as before. No new queries, loops, blocking I/O or allocations in a hot path.

Deferred-findings section is sound. I spot-checked the deploy-archive.ts:96 sleep-vs-delay claim: test/deploy-archive-logs.test.ts drives polls off vitest fake timers, which do not intercept node:timers/promises, so the author's "tried, reverted" is correct and the deferral is the right call.

Verdict

approved — 0 Critical findings; two Suggestions, neither blocking. (Informational: the green checkmark is a separate human action, and the PR has in any case already merged as 1398146.)